Reviewable status: 0 of 11 files reviewed, 8 unresolved discussions (waiting on @buggmagnet)

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 215 at r1 (raw file):

            }
        )
        addCustomListAction.accessibilityIdentifier = AccessibilityIdentifier.addNewCustomListButton

Since the property is of AccessibilityIdentifier it can be simplified to just .addNewCustomListButton. I saw you used this syntax in some places but some not.

ios/MullvadVPNUITests/CustomListsTests.swift line 17 at r1 (raw file):

        SelectLocationPage(app)
            .openCustomListsActions()

I think it's a very useful convention to only have functions doing one user action each in test case level. It makes it easy to glance the test case code and see what the test is doing without having to look in multiple functions. This way it's easier to make sure the tests are testing what they should, and it reduces(or eliminates) the need of documenting test case user actions elsewhere.

So for example openCustomListsActions() could split into:

.scrollToCustomListsSection()
.tapCustomListsEllipsisButton()

ios/MullvadVPNUITests/CustomListsTests.swift line 24 at r1 (raw file):

        CustomListPage(app)
            .verifyCreateButtonIs(enabled: false)
            .editCustomList(name: customListName)

Same comment as a above, IMO it's very helpful to not combine user actions in methods but doing one user action per function in test case level. So this could be for example

.tapNameTextField()
.enterText(customListName)

(The Page superclass have some generic functions like enterText())

After entering the custom list name the keyboard is still visible. I'm not sure what a real user is more likely to do here and what's better to test but could either continue navigation without dismissing keyboard or dismiss keyboard first. Not sure if it's important to dismiss keyboard here but the page superclass Page has a function dimissKeyboard() if deciding to dismiss keyboard.

ios/MullvadVPNUITests/XCUIElementQuery+Extensions.swift line 19 at r1 (raw file):

    subscript(key: AccessibilityIdentifier) -> XCUIElement {
        self[key.rawValue]
    }

nit: super neat 👍👍

ios/MullvadVPNUITests/Pages/CustomListPage.swift line 19 at r1 (raw file):

    }

    @discardableResult

nit: Seems like swiftformat allows both @discardableResult on same line as function declaration and on separate line above. I have doing it on same line previously but don't have any strong preference, kinda like both. Should we try to agree on one way of doing it and sticking to it?

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 52 at r1 (raw file):

    func scrollToCustomListsSection() -> Self {
        let selectLocationTableView = app.tables[AccessibilityIdentifier.selectLocationTableView]
        selectLocationTableView.swipeDown(velocity: XCUIGestureVelocity(floatLiteral: 9999))

XCUIGestureVelociy have some constants that make it easier to read out how fast the velocity is(.default, .slow, .fast) https://developer.apple.com/documentation/xctest/xcuigesturevelocity#3638600

9999 pixels per second seems very fast. I think if not adding much time or complexity it's nice to do it as human-like as possible. Another fast and reliable way of scrolling to the top is tapping the iOS status bar.

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 56 at r1 (raw file):

    }

    func tapAddNewCustomList() {

I have made all page functions chainable by returning self regardless of whether another action on the same page might follow the function invocation or not, because the app's flow might change and I've been treating it as a protocol to be followed even though there is no such protocol. An argument could be made that it is invalid to continue chaining after these functions though, so your approach here also makes sense. What's your thoughts?

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 69 at r1 (raw file):

    override func setUp() {
        continueAfterFailure = false
        app.launchArguments = ["DisableAnimations"]

Should we disable animations? It will make the tests run faster but at the cost of the app being tested differing slightly from the app users use.

Reviewable status: 0 of 11 files reviewed, 8 unresolved discussions (waiting on @niklasberglund)

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 215 at r1 (raw file):

Yes that's correct, I want to do another pull request where we only modify all those AccessibilityIdentifier in one swoop, because otherwise the changes in the codebase will be harder to review

ios/MullvadVPNUITests/CustomListsTests.swift line 17 at r1 (raw file):

Done.

ios/MullvadVPNUITests/CustomListsTests.swift line 24 at r1 (raw file):

On principle I agree with you, but I'd like to argue that in this specific case, we are doing one action only.
I agree with the general idea that we want each action to do one thing only however, one cannot edit a field without tapping it first.
Imagine we had to write a test for a form with several text fields, it would be extremely redundant to see the following

form.tapTextField1()
form.enterText(someText1)

form.tapTextField2()
form.enterText(someText2)

form.tapTextField3()
form.enterText(someText3)

This is also why I tried to make the method name read as naturally as possible, there is no possible confusion here as to what the single action is doing.

ios/MullvadVPNUITests/Pages/CustomListPage.swift line 19 at r1 (raw file):

That's a good point, I'll put it on the same line

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 52 at r1 (raw file):

This was discussed offline, I will try to find a way to tap the status bar instead.

EDIT:
It used to be possible before iOS 12, since then, the status bar is not considered part of the application's UI anymore, therefore it's not a good method.

I will leave it like this as it's reliable enough.

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 56 at r1 (raw file):

I've added chaining when I needed it, but I think it's useful as a convention to have, especially because we can use @discardableResult
I will add it too here

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 69 at r1 (raw file):

That's true. Also I noticed that even with animation disabled, we still show some of the views with an animation, which is not great.
Ultimately, we want the UITests to run as fast as possible, and since animations are not really easy to verify with UITests in the first place, I think it's okay to have them disabled in UITests as well.

Reviewable status: 0 of 11 files reviewed, 8 unresolved discussions (waiting on @buggmagnet)

ios/MullvadVPN/Coordinators/LocationCoordinator.swift line 215 at r1 (raw file):

Sounds good 👍

ios/MullvadVPNUITests/CustomListsTests.swift line 24 at r1 (raw file):

Agree with you that it is a quite specific action to enter a text into a textfield, but there is room for confusion I think. To me it's not obvious that editCustomList(name: customListName) enters this name for the custom list by just reading the code. To edit a custom list could mean other things too.

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 52 at r1 (raw file):

Then the current solution sounds good to me 👍

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 69 at r1 (raw file):

Personally I'm not really for or against disabling animations. Both have their benefits. Check with the team after standup?

Reviewable status: 0 of 18 files reviewed, 7 unresolved discussions (waiting on @niklasberglund)

ios/MullvadVPNUITests/Pages/SelectLocationPage.swift line 52 at r1 (raw file):

Done.

ios/MullvadVPNUITests/Test base classes/BaseUITestCase.swift line 69 at r1 (raw file):

We decided during the standup to disable animations for UI Tests

Reviewable status: 0 of 18 files reviewed, 7 unresolved discussions (waiting on @niklasberglund)

ios/MullvadVPNUITests/CustomListsTests.swift line 24 at r1 (raw file):

This was discussed offline, we will rename this renameCustomList

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @buggmagnet)

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 143 at r3 (raw file):

        checkboxButton.addTarget(self, action: #selector(toggleCheckboxButton(_:)), for: .touchUpInside)
//        checkboxButton.accessibilityIdentifier

Should this be removed?

ios/MullvadVPNUITests/CustomListsTests.swift line 28 at r3 (raw file):

            .tapWhereStatusBarShouldBeToScrollToTopMostPosition()

        XCTAssertTrue(app.staticTexts[customListName].exists)

nit: it would be nice to specifically look for custom list cell with this name. In the current implementation I think this check is fine, but there is a risk that the custom list name in the future could appear elsewhere in the view hierarchy.

ios/MullvadVPNUITests/CustomListsTests.swift line 38 at r3 (raw file):

        createCustomList(named: customListName)
        workaroundOpenCustomListMenuBug()
        deleteCustomList(named: customListName)

Since deleting the custom list is part of the actual test I suggest breaking it out to represent user interactions. IMO automated test cases should read like manual test cases. If a manual test says "delete custom list" its leaving much room for interpretation of the tester. Maybe later for example it's possible to delete by swiping left on the cell then tapping a delete button. It's useful to see how the item is deleted directly in the test case.

ios/MullvadVPNUITests/CustomListsTests.swift line 43 at r3 (raw file):

            .tapWhereStatusBarShouldBeToScrollToTopMostPosition()

        XCTAssertFalse(app.staticTexts[customListName].exists)

When running testDeleteCustomList its failing here for me because the select location table view is in the view hierarchy in the background and has not been updated. See screenshot where I created custom lists "1" and "2" then removed "1". I think the assert should be more specific and look for custom list cell title with this text.

ios/MullvadVPNUITests/CustomListsTests.swift line 53 at r3 (raw file):

        createCustomList(named: customListName)
        workaroundOpenCustomListMenuBug()
        startEditingCustomList(named: customListName)

Same comment as above(https://reviewable.io/reviews/mullvad/mullvadvpn-app/6148#-Nw4lADRASPX17jV50Ow) since this is part of the actual test. In setup and teardown it's good to use functions combining actions.

ios/MullvadVPNUITests/CustomListsTests.swift line 64 at r3 (raw file):

        ListCustomListsPage(app)
            .finishEditingCustomLists()

The function does one user action but the name is ambiguous about specifically how editing custom lists is finished. Suggest for example tapDoneButton()

ios/MullvadVPNUITests/CustomListsTests.swift line 69 at r3 (raw file):

        workaroundOpenCustomListMenuBug()
        deleteCustomList(named: customListName)

Teardown code should be in teardown when possible so that it always run. addTeardownBlock is useful when a teardown is specific to a test and not all tests in the suite. Could for example do something like this at the top if this test:

addTeardownBlock {
    TunnelControlPage(app)
        .tapSelectLocationButton()
    
    SelectLocationPage(app)
            .tapWhereStatusBarShouldBeToScrollToTopMostPosition()
            .tapCustomListEllipsisButton()

    deleteCustomList(named: customListName)
}

ios/MullvadVPNUITests/CustomListsTests.swift line 86 at r3 (raw file):

    }

    func workaroundOpenCustomListMenuBug() {

Should this be addressed in the app? It might not only affect tests but also users relying on accessibility features.

ios/MullvadVPNUITests/Pages/Page.swift line 51 at r3 (raw file):

    @discardableResult func tapWhereStatusBarShouldBeToScrollToTopMostPosition() -> Self {
        app.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0)).tap()

Will this tap be in the upper left corner? Wondering if it can cause issues in some scenarios for example with dynamic island or when iOS is showing that you're sharing location data, are in a call or similar. Then it's a tappable button in the upper left corner.

Would this tap in the middle of the status bar?
app.coordinate(withNormalizedOffset: CGVector(dx: app.frame.width/2, dy: 10)).tap()

I think status bar always(or in almost all cases?) are 20 points high

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @niklasberglund)

ios/MullvadVPN/View controllers/SelectLocation/LocationCell.swift line 143 at r3 (raw file):

yes it should 🙈

ios/MullvadVPNUITests/CustomListsTests.swift line 38 at r3 (raw file):

I hear what you're saying, but we're using this function in all the tests so far, and it's less error prone to have one function that does it than repeat all the steps in all the tests.

We are not supporting swipe to delete yet, so we can modify the code when we do

ios/MullvadVPNUITests/CustomListsTests.swift line 43 at r3 (raw file):

This was discussed offline, the reason was because there already was a custom list present, which meant the navigation was not what the framework expected.

ios/MullvadVPNUITests/CustomListsTests.swift line 64 at r3 (raw file):

Done

ios/MullvadVPNUITests/CustomListsTests.swift line 86 at r3 (raw file):

Currently accessibility is broken in LocationSelection, let's discuss this with the team.

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @niklasberglund)

ios/MullvadVPNUITests/Pages/Page.swift line 51 at r3 (raw file):

I hear what you're saying, but why would a phone used for automation testing be doing that ?

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

ios/MullvadVPNUITests/Pages/Page.swift line 51 at r3 (raw file):

Could we not trigger the same automation that is used when you use accesibility tools to swipe to the top?
Ultimately, I don't feel like this should be a blocker, but ensuring that these tests run as smooth as possible on dev phones and test phones alike should be a priority.

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @niklasberglund)

ios/MullvadVPNUITests/Pages/Page.swift line 51 at r3 (raw file):

We could also go with Niklas' suggestion and be done with it -if it works.

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

ios/MullvadVPNUITests/CustomListsTests.swift line 38 at r3 (raw file):

Agree, I was just concerned it might not be easy to get an overview but it is easy to get an overview since it's just a click-through away and then all the user actions are there 👍

ios/MullvadVPNUITests/Pages/Page.swift line 51 at r3 (raw file):

With the phone dedicated to testing it shouldn't be an issue but could be an issue with developers' phones. I don't think this is very important and wanna downgrade this to a nit 😊

Reviewable status: 0 of 18 files reviewed, 16 unresolved discussions (waiting on @buggmagnet and @pinkisemils)